feat(worker): add aggregate ratings API on D1 (test bucket) - #427
Closed
richardthe3rd wants to merge 8 commits into
Closed
feat(worker): add aggregate ratings API on D1 (test bucket)#427richardthe3rd wants to merge 8 commits into
richardthe3rd wants to merge 8 commits into
Conversation
First step towards an online "my festival". Clients submit a drink rating and get back the shared aggregate (count + average + their own rating). - New /v1/ratings endpoints on the existing proxy worker: POST/DELETE upsert/remove a device's rating, GET single + batch aggregates. - D1-backed storage with upsert semantics (one row per device/drink) so re-rating never inflates counts. Anonymous device_id now; user_id column reserved for the sign-in upgrade. - Every row/query scoped by a `bucket` so test traffic stays isolated from production data; bucket derived from origin, overridable via RATINGS_BUCKET. - CORS extended to POST/DELETE. - Full vitest coverage against a simulated local D1 (no real database needed): pure-helper unit tests plus integration tests for upsert, aggregation, validation, deletion and bucket isolation. 78 worker tests pass. The wrangler.toml database_id is a placeholder; local dev and tests use a simulated D1. README documents the endpoints and the one-time `wrangler d1 create` / migrations-apply provisioning before first deploy.
A yes/no "would recommend" signal, separate from the star rating, so each drink can surface a "% would recommend". - New /v1/recommendations endpoints mirroring ratings (POST/DELETE upsert, GET single + batch). Aggregate reports total responses, "yes" count and the recommend percentage, plus the caller's own answer. - Stored in a new `recommendations` table in the same D1 database, with the same per-device upsert and bucket-isolation model. - Extracted shared bucket/id-validation/JSON/REST-routing plumbing into shared.js so ratings and recommendations stay thin; ratings refactored to consume it (behaviour unchanged). - 19 new tests (pure helpers + integration for upsert, aggregation, validation, deletion, bucket isolation). 97 worker tests pass. README documents the new endpoints; migration 0002 adds the table.
Rework the /v1 API to conform to the proto contract and Google's AIPs.
BREAKING CHANGE: replaces the flat POST/DELETE /v1/ratings endpoints with
resource-oriented routes. Nothing consumes them yet (no client, placeholder
DB), so this is a safe pre-launch change.
- Resource names: PATCH/GET/DELETE on
/v1/festivals/{f}/drinks/{d}/ratings/{device} (and .../recommendations/...).
- Upsert via PATCH with allow_missing semantics (AIP-134); bodyless DELETE
that is NOT_FOUND when absent (AIP-135).
- Read aggregates as RatingSummary / RecommendationSummary resources:
GET .../{f}/ratingSummaries/{d} and a paginated list
GET .../{f}/ratingSummaries (page_size/page_token/next_page_token +
total_size, keyset cursor) (AIP-158).
- Structured google.rpc.Status errors with ErrorInfo reason+domain (AIP-193).
- RFC3339 update_time; camelCase resource fields matching the proto JSON
mapping; dropped redundant your_* (client is local-first and knows its own).
- Generic family engine in shared.js drives both resources; CORS now allows
GET/PATCH/DELETE; unknown /v1 routes 404 instead of proxying upstream.
- 87 worker tests pass.
Rebases the ratings/recommendations worker from PR #426 onto current main and updates the implementation to conform to the v1alpha proto contract merged in PR #425. Changes from the original design: - URL prefix: /v1/ → /v1alpha/ - Separate `ratings/{device}` + `recommendations/{device}` collections replaced by a single `Review` singleton at `drinks/{d}/review` - Device ID moves from the URL to the `X-Device-Id` request header; it no longer appears in resource names (auth-upgrade transparent) - Separate `ratingSummaries` + `recommendationSummaries` merged into `reviewSummaries` (combined ratingCount + responseCount/recommendRate) - `starRating` + `wouldRecommend` signals independently nullable; `updateMask` in the PATCH body allows updating one without clearing the other - DB schema: two tables → single `reviews` table with nullable columns New routes: GET/PATCH/DELETE /v1alpha/festivals/{f}/drinks/{d}/review GET /v1alpha/festivals/{f}/reviews GET /v1alpha/festivals/{f}/reviewSummaries[/{d}] Implementation: - reviews.js replaces ratings.js + recommendations.js - shared.js retains utility functions (bucket, errors, pagination) - Single migration: 0001_create_reviews_table.sql - 85 vitest tests pass (pure helpers, upsert, get/delete, list, summaries, bucket isolation, missing header, routing) - CORS: allow X-Device-Id header alongside Content-Type https://claude.ai/code/session_01VVTCRjdHqcTJCVEjEVQV6C
Populated by mise during toolchain install (buf 1.70.0 via aqua backend). https://claude.ai/code/session_01VVTCRjdHqcTJCVEjEVQV6C
Convert reviews.js → reviews.ts and shared.js → shared.ts. Response bodies (Review, ReviewSummary, ListReviewsResponse, etc.) are now typed against the generated src/api-types.ts (proto → OpenAPI → openapi-typescript), so a field rename or type change in the proto surfaces as a compile error in the implementation. - types: Review, ReviewSummary, ListReviewsResponse, ListReviewSummariesResponse imported from components["schemas"][...] in the generated api-types.ts - Env interface (RATINGS_DB: D1Database, RATINGS_BUCKET?) centralised in shared.ts - D1 row shapes (ReviewRow, SummaryRow, etc.) typed for all queries - tsc --noEmit passes clean (strict mode, moduleResolution: bundler) - 85 vitest tests still pass - package.json: add typecheck script; tsconfig.json added - mise.toml: test:worker now runs tsc before vitest Regenerate types after proto changes: MISE_ENV=dev ./bin/mise run proto:generate MISE_ENV=dev ./bin/mise run proto:clients:types https://claude.ai/code/session_01VVTCRjdHqcTJCVEjEVQV6C
Add a proto job to CI that runs buf lint and buf breaking on every PR that touches proto/. Breaking change detection uses FILE stability level (configured in proto/buf.yaml), appropriate for v1alpha APIs — catches source-breaking changes to generated code while allowing additive changes. buf breaking only runs on pull_request events (bufbuild/buf-action skips it on push to main where the PR is already merged). Lint runs on both. Switch breaking.use from FILE to WIRE_JSON_COMPATIBLE in proto/buf.yaml when the API graduates from v1alpha to v1 stable. https://claude.ai/code/session_01VVTCRjdHqcTJCVEjEVQV6C
- Validate drinkId before getReviewSummary to prevent an injection path through the resource-name segments (INVALID_RESOURCE_NAME 400) - Reject unknown updateMask fields rather than silently ignoring them (UNKNOWN_FIELD_MASK 400), matching AIP-134 contract guarantees - Eliminate post-write readRow() in upsert: compute finalStarRating / finalRecommend before writing and build the response from those values, removing one DB round trip and closing a TOCTOU race where a concurrent DELETE between write and re-read caused a non-null assertion crash Test: adds UNKNOWN_FIELD_MASK case; all 86 tests pass https://claude.ai/code/session_01VVTCRjdHqcTJCVEjEVQV6C
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the existing Cloudflare proxy worker with a new D1-backed “my festival” review API (review upsert/read/delete plus aggregate summaries), adds local/test-only D1 migration wiring for vitest, and introduces proto lint/breaking checks in CI.
Changes:
- Add
/v1alpha/...review + reviewSummary endpoints backed by a D1reviewstable (bucket-scoped for test/prod separation). - Extend worker test setup to apply SQL migrations to a simulated D1 and add comprehensive vitest coverage for routing/validation/aggregation/pagination.
- Add TypeScript typechecking for the worker and a new CI job to lint/break-check
proto/**with buf.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| mise.toml | Updates worker test task to run tsc typecheck before vitest. |
| mise.dev.lock | Adds buf tool lock entry (used by new proto checks). |
| cloudflare-worker/wrangler.toml | Declares D1 binding + placeholder database_id and migrations dir. |
| cloudflare-worker/worker.js | Routes /v1alpha/* to new handler; expands CORS preflight methods/headers. |
| cloudflare-worker/vitest.config.js | Loads D1 migrations at config time and wires setup file to apply them. |
| cloudflare-worker/tsconfig.json | Adds strict TS config for worker typechecking. |
| cloudflare-worker/test/reviews.test.js | New integration + unit tests for helpers, CRUD, aggregates, pagination, bucket isolation. |
| cloudflare-worker/test/cors.test.js | Updates expectations for expanded preflight allowlists. |
| cloudflare-worker/test/apply-migrations.js | Applies SQL migrations to the simulated per-test D1. |
| cloudflare-worker/shared.ts | Adds bucket resolution, structured errors, and page token helpers. |
| cloudflare-worker/reviews.ts | Implements review + aggregate API handlers and D1 queries. |
| cloudflare-worker/README.md | Documents the v1alpha API and D1 provisioning steps. |
| cloudflare-worker/package.json | Adds typecheck script and TS/workers-types deps. |
| cloudflare-worker/package-lock.json | Locks added dependencies (TS + workers-types). |
| cloudflare-worker/migrations/0001_create_reviews_table.sql | Creates reviews table + aggregate index. |
| .github/workflows/ci.yml | Adds proto change detection output and buf lint/breaking CI job. |
Files not reviewed (1)
- cloudflare-worker/package-lock.json: Generated file
Comment on lines
16
to
+19
| // Import festivals data directly - copied from data/festivals.json during build | ||
| import festivalsData from "./festivals.json"; | ||
| import { handleReviews } from "./reviews.js"; | ||
| import { errorResponse } from "./shared.js"; |
Comment on lines
+21
to
+32
| import type { components } from "./src/api-types"; | ||
| import { | ||
| type CorsHeaders, | ||
| type Env, | ||
| resolveBucket, | ||
| rfc3339, | ||
| jsonResponse, | ||
| errorResponse, | ||
| encodePageToken, | ||
| decodePageToken, | ||
| resolvePageSize, | ||
| } from "./shared.js"; |
Comment on lines
+7
to
+14
| import worker from "../worker.js"; | ||
| import { | ||
| isProductionOrigin, | ||
| resolveBucket, | ||
| resolvePageSize, | ||
| encodePageToken, | ||
| decodePageToken, | ||
| } from "../shared.js"; |
Comment on lines
+20
to
+22
|
|
||
| import type { components } from "./src/api-types"; | ||
| import { |
Comment on lines
+121
to
+140
| function parseV1alphaPath(pathname: string): string[] | null { | ||
| if (pathname !== "/v1alpha" && !pathname.startsWith("/v1alpha/")) return null; | ||
| return pathname | ||
| .slice("/v1alpha/".length) | ||
| .split("/") | ||
| .filter((s) => s.length > 0) | ||
| .map((s) => decodeURIComponent(s)); | ||
| } | ||
|
|
||
| /** Route a request, or return null if the path doesn't match any review route. */ | ||
| export async function handleReviews( | ||
| request: Request, | ||
| url: URL, | ||
| env: Env, | ||
| corsHeaders: CorsHeaders, | ||
| ): Promise<Response | null> { | ||
| const segments = parseV1alphaPath(url.pathname); | ||
| if (!segments || segments[0] !== "festivals" || segments.length < 3) { | ||
| return null; | ||
| } |
Comment on lines
+98
to
+106
| export function decodePageToken(token: string | null): string | null | undefined { | ||
| if (!token) return null; | ||
| try { | ||
| const b64 = token.replace(/-/g, "+").replace(/_/g, "/"); | ||
| return decodeURIComponent(escape(atob(b64))); | ||
| } catch { | ||
| return undefined; // signal "invalid token" | ||
| } | ||
| } |
Comment on lines
+19
to
+28
| export function isProductionOrigin(origin: string): boolean { | ||
| return origin === "https://cambeerfestival.app"; | ||
| } | ||
|
|
||
| export function resolveBucket(origin: string, env: Partial<Env>): string { | ||
| if (env && typeof env.RATINGS_BUCKET === "string" && env.RATINGS_BUCKET) { | ||
| return env.RATINGS_BUCKET; | ||
| } | ||
| return isProductionOrigin(origin) ? "prod" : "test"; | ||
| } |
Comment on lines
+95
to
+105
| ### "My festival" API (v1alpha) | ||
|
|
||
| Personal drink reviews and shared aggregates, backed by D1 (SQLite). The first | ||
| step towards an online "my festival". The API is resource-oriented following | ||
| [Google's AIPs](https://google.aip.dev) — the proto contract is in `proto/` | ||
| and an OpenAPI spec can be generated from it (see `proto/README.md`). | ||
|
|
||
| Writes are local-first on the client; the server holds the shared aggregate. | ||
| Every row and query is scoped by a `bucket` (`test` or `prod`, derived from the | ||
| request origin; only `https://cambeerfestival.app` → `prod`) so test traffic | ||
| never mixes with production data. A `RATINGS_BUCKET` worker var can pin it. |
Comment on lines
268
to
274
| return new Response(null, { | ||
| status: 204, | ||
| headers: { | ||
| ...getCorsHeaders(request), | ||
| "Access-Control-Allow-Methods": "GET, OPTIONS", | ||
| "Access-Control-Allow-Headers": "Content-Type", | ||
| "Access-Control-Allow-Methods": "GET, PATCH, DELETE, OPTIONS", | ||
| "Access-Control-Allow-Headers": "Content-Type, X-Device-Id", | ||
| "Access-Control-Max-Age": maxAge, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First step towards an online "my festival". Clients submit a drink rating
and get back the shared aggregate (count + average + their own rating).
POST/DELETE upsert/remove a device's rating, GET single + batch aggregates.
re-rating never inflates counts. Anonymous device_id now; user_id column
reserved for the sign-in upgrade.
bucketso test traffic stays isolated fromproduction data; bucket derived from origin, overridable via RATINGS_BUCKET.
pure-helper unit tests plus integration tests for upsert, aggregation,
validation, deletion and bucket isolation. 78 worker tests pass.
The wrangler.toml database_id is a placeholder; local dev and tests use a
simulated D1. README documents the endpoints and the one-time
wrangler d1 create/ migrations-apply provisioning before first deploy.